[WIP] test: run root test with build flag #500
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hmmm, actually looking at this now, it'd be nice to keep all the tests in the place they belong. I wonder if we can solve this a much simpler way. When I just run the tests as root with There are a few failures from tests that expect a permissions failure or something, and don't get the failure as root, so the test fails, for example: However, those would be easy to tweak, or even Then we could just run the root tests with a single command like this: |
|
Closing this PR according to the above discussion. We will run all tests with root so that all tests can be kept together in the same place where the package is. |
IronCore864
added a commit
that referenced
this pull request
Sep 23, 2024
Background: Previously, when adding a new root test, we had to modify the GitHub Actions workflow to include the newly added test, which is some extra operational overhead. Per discussion [here](#500 (comment)), we agreed to run all tests with root and skip those tests that would fail with root (checking permissions and stuff), and this PR skips tests that would fail when running with root and made the GitHub Actions workflow for root tests much easier.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
5 tests moved to build flag
internals/daemon:TestUserGroupinternals/daemon:TestUserIDGroupIDinternals/daemon:TestWriteUserGroupRealinternals/daemon:TestMakeDirsUserGroupRealinternals/osutil:TestMakeParentsChmodAndChownTestUserGroupandTestUserIDGroupIDare moved into a new fileinternals/daemon/api_exec_root_test.go, and since they require some setup/teardown, I created aTestMainfunc there.TestWriteUserGroupRealandTestMakeDirsUserGroupRealare moved into a separate new fileinternals/daemon/api_files_root_test.go, they don't need setup, but because it's in the same package, there is a problem: theTestMainfrom the other file still runs for these tests. The tests won't fail, but doing a non-required setup feels wrong.The only solution I can think of is to create a new folder, something like the integration tests and put
TestUserGroupandTestUserIDGroupIDin one subfolder/package, and putTestWriteUserGroupRealandTestMakeDirsUserGroupRealinto a different subfolder/package, so that theTestMainfunc from one won't interfere with the other. Any better solution here?1 test not moved to build flag:
internals/overlord/servstate:TestUserGroupThis is because the setup/teardown for this test is complicated, moving it to another new file basically means copy-pasting most parts of the existing setup/teardown code and rewriting them not using a suite. I did part of this and found it was too complicated so I gave it up and reverted the changes. Need some input here about this test.